Conversation
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
From a conceptual point of view, do we need associated token account clients at all? The legacy and generated JS packages for token bundle the ATA instructions, and the token client does ATA stuff too
Ah, didn't realize it's handled independently over there! Sounds better to align on those clients. Also noticed that there's even an IDL in the token program repo. Not quite sure how that was made, but think there's an argument that the codama-macros in p-ata should generate the canonical IDL. It also ensures it's up-to-date. Then external programs can consume it and generate the clients. What do you think? |
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
I think it makes sense to generate the IDL! A couple more comments
lorisleiva
left a comment
lorisleiva
left a comment
There was a problem hiding this comment.
From a conceptual point of view, do we need associated token account clients at all?
Hmm that's a good question. This definitely needs its own IDL because in the future it would be nice to have Codama auto-load linked program items (such as an ATA PDA) using the Program Metadata program. But as far as having clients, as you said we currently inline that content on token programs which is more practical for devs so probably not worth the overhead of maintaining yet another set of libraries.
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks good to me, but do get @lorisleiva 's approval too
| @echo "No JavaScript clients to generate" | ||
|
|
||
| generate-idl-%: | ||
| @cargo install --locked --version =0.7.3 codama-cli |
There was a problem hiding this comment.
nit: for the future, rather than installing in the makefile target, it would be better for the generic action to install required tooling -- for example, the spellcheck job installs cargo-spellcheck and the audit job installs cargo-audit: https://github.com/solana-program/actions/blob/cf2614dd80f3646bc730ea75be989e4ed61383ae/.github/workflows/main.yml#L150
However, maybe we should expect the codama-cli to change a lot over the next year or two. When we feel that it's "stable", then we can add the installation in the generic action
There was a problem hiding this comment.
Out of curiosity, why not set codama-cli as a dev dependency on the workspace?
There was a problem hiding this comment.
As far as I know, Rust doesn't really have a mechanism for bins at the workspace level, similar to the pnpm <BIN> commands in JS. You install the bin for the user using cargo install and that's it.
(Or you use something like a nix shell, but that's a topic for another day 😁 )
There was a problem hiding this comment.
However, maybe we should expect the codama-cli to change a lot over the next year or two. When we feel that it's "stable", then we can add the installation in the generic action
Makes sense! Though, if we shipped and used tagged versions of solana-program/actions, breaking changes in those jobs wouldn't affect consumers. May be worth revisiting later.
There was a problem hiding this comment.
Maybe we can do that if you promise to do all the downstream repo upgrades for every bump 😉
3 participants
Added a codama pipeline for the pinocchio ATA crates to generate an IDL. Clients still generated externally in token program.
Note: does not modify the original non-pinocchio crates